Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[geos] Relocatable geos-config; dynamic builds on Unix #17616

Merged
merged 9 commits into from
May 26, 2021

Conversation

dg0yt
Copy link
Contributor

@dg0yt dg0yt commented May 1, 2021

I still need to test the latest state with my gdal branch, but it is ready for vcpkg CI. The question in #17607 are related, too.

@dg0yt dg0yt changed the title [geos] Provide geos-config; fix dynamic builds on Unix [geos] Relocatable geos-config; dynamic builds on Unix May 1, 2021
@dg0yt dg0yt marked this pull request as ready for review May 2, 2021 04:04
@JackBoosY JackBoosY self-assigned this May 2, 2021
@JackBoosY JackBoosY added category:port-bug The issue is with a library, which is something the port should already support category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist labels May 2, 2021
@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label May 2, 2021
string(REGEX REPLACE "(-lgeos(_c)?)d?( |\n)" "\\1d\\3" GEOS_CONFIG "${GEOS_CONFIG}")
file(WRITE "${CURRENT_PACKAGES_DIR}/debug/bin/geos-config" "${GEOS_CONFIG}")
file(MAKE_DIRECTORY ${CURRENT_PACKAGES_DIR}/share/${PORT})
file(RENAME ${CURRENT_PACKAGES_DIR}/debug/bin/geos-config ${CURRENT_PACKAGES_DIR}/share/${PORT}/geos-config-debug)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you give an example of how this would be consumed? My understanding is that xyz-config scripts like this are usually referred to like "${XYZ_ROOT}/xyz-config" and so having xyz-config-debug won't be directly usable. Instead, we should provide debug/share/geos/geos-config and set GEOS_ROOT to debug/share/geos when in debug mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gdal configure says:

  --with-geos=ARG         Include GEOS support (ARG=yes, no or geos-config
                          path)

and similar for curl, kea, sfcgal.

So this is to be consumed in the gdal portfile by:

    list(APPEND OPTIONS_RELEASE --with-geos=${CURRENT_INSTALLED_DIR}/share/geos/geos-config)
    list(APPEND OPTIONS_DEBUG --with-geos=${CURRENT_INSTALLED_DIR}/share/geos/geos-config-debug)

I tested this locally, but this just one out of a pile of pending changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

provide debug/share/geos/geos-config and set set GEOS_ROOT to debug/share/geos

If I adopt this: Outside vcpkg, the <package>_ROOT variable generally (e.g. in CMake find modules) means the installation prefix, i.e. ${CURRENT_INSTALLED_DIR}.
As a consumer in a port file, I would still have to assign the roots to per-build-type parameters because the build types are handled deeper down in the cmake call stack.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

provide debug/share/geos/geos-config

I tried that when preparing the PR, and I verified today: It leads into a build error:

-- Performing post-build validation
/debug/share should not exist. Please reorganize any important files, then use
    file(REMOVE_RECURSE "${CURRENT_PACKAGES_DIR}/debug/share")

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which file is in ${CURRENT_PACKAGES_DIR}/debug/share?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which file is in ${CURRENT_PACKAGES_DIR}/debug/share?

None with the current PR. @ras0219-msft proposed to have debug/share/geos/geos-config, and I reported what would be the result.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The alternative to fixing geos-config, curl-config etc. is forcing the consumers to use pkg-config where possible, by removing these files. Consumers also need to know whether to ask the scripts for static or standard flags, and this is solved for PKG_CONFIG.
E.g. port gdal will need a number of patches anyway, e.g. to use pkg-config for tiff.

@JackBoosY JackBoosY removed the info:reviewed Pull Request changes follow basic guidelines label May 5, 2021
@JackBoosY
Copy link
Contributor

Ping @ras0219-msft for review this PR again.

@dg0yt
Copy link
Contributor Author

dg0yt commented May 8, 2021

More side notes:

  • This port sets the debug postfix ('d'). This might be contrary to the current maintainer guidelines. git grep geosd finds a small number of ports which depend on this.
  • pc files do not carry the debug postfix.
  • pc files are not passed to fixup. (Prefix pointing to packages.)
  • Static linking dependencies are incomplete in the pc files.

@dg0yt
Copy link
Contributor Author

dg0yt commented May 12, 2021

Will add pc file handling soon.

dg0yt added 4 commits May 22, 2021 07:30
Fixup pc files.
Add -lgeos to Libs.private to satisfy common usage requirement
(already present in geos-config).
Install executable geos-config into tools.
Rewrite geos-config relocatability as patch.
@dg0yt
Copy link
Contributor Author

dg0yt commented May 22, 2021

New changes:

  • pc files rectified.
  • geos-config moved to tools/bin and tools/debug/bin.
  • Ported to vcpkg_configure_....
  • Building of benchmarks disabled.
  • Usage instructions added.

@dg0yt dg0yt marked this pull request as ready for review May 22, 2021 14:59
@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label May 24, 2021
@strega-nil-ms
Copy link
Contributor

Cool, LGTM! Thanks @dg0yt :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants